GH-98831: Add macro and op and their implementation to DSL#99495
GH-98831: Add macro and op and their implementation to DSL#99495gvanrossum merged 21 commits intopython:mainfrom
macro and op and their implementation to DSL#99495Conversation
4cdb25f to
f7895ac
Compare
(But this needs to be cleaned up for maintainability.)
Also: - Add helpers for indent management; - Reformat with black.
598e71a to
1aafac8
Compare
This was popping the common items off the stack.
|
|
||
| TARGET(WITH_EXCEPT_START) { | ||
| PyObject *val = PEEK(1); | ||
| PyObject *lasti = PEEK(3); |
There was a problem hiding this comment.
This warning is annoying, the variable is used in an assert() call.
There was a problem hiding this comment.
Maybe stick a (void)lasti; // <helpful comment> near the assert?
There was a problem hiding this comment.
Oooh, I didn't realize you could do that. Will do.
|
FWIW I requested a buildbot run for GH-99601, which is 2 commits ahead of this one. If that passes I will assume it would pass here too, without actually trying it (leave some resources for others). |
brandtbucher
left a comment
There was a problem hiding this comment.
Thanks for your patience! I think this is a good step forward.
|
|
||
| TARGET(WITH_EXCEPT_START) { | ||
| PyObject *val = PEEK(1); | ||
| PyObject *lasti = PEEK(3); |
There was a problem hiding this comment.
Maybe stick a (void)lasti; // <helpful comment> near the assert?
| if ceffect.size == 1: | ||
| # There is no read_u16() helper function. | ||
| f.write(f"*(next_instr + {cache_offset});\n") | ||
| else: |
There was a problem hiding this comment.
Eh, should we just add a read_u16 function to keep things simple? They're not really special anymore now that we've ditched the structs, and we've shown that it would compile the same.
There was a problem hiding this comment.
Oh, good idea. Will do.
| PyObject *_tmp_1 = PEEK(2); | ||
| PyObject *_tmp_2 = PEEK(1); |
There was a problem hiding this comment.
Just a note on the generated code... this looks sort of odd to me. _tmp_1 is the second item on the stack, and is used by the second instruction part. _tmp_2 is the first item on the stack and is used by the first instruction part.
Not a big deal, but it seems like they should be named in the order they are actually used, like the other superinstructions? My brain took a second to parse and re-parse the generated code a couple of times.
There was a problem hiding this comment.
I agree this is annoying, but it's hard to get the numbering consistent since there may be variables that aren't popped off the stack but are pushed back onto it. I could number them in reverse order, but there could still be cases where the numbering would be off.
E.g. for stack effect (a, b -- c) we'd see something like
PyObject *_tmp_3 = PEEK(2);
PyObject *_tmp_2 = PEEK(1);
PyObject *_tmp_1;
{
PyObject *a = _tmp_3;
PyObject *b = _tmp_2;
PyObject *c;
<c = a + b>
_tmp_1 = c;
}
STACK_GROW(1);
PEEK(1, _tmp_1);
At some point I considered using letters instead of number the temp variables, but that wouldn't help much.
Once we have a register VM it'll be less of an issue. :-)
|
|
||
| @dataclass | ||
| class InstHeader(Node): | ||
| kind: Literal["inst", "op"] |
There was a problem hiding this comment.
The "kind" tag used here and elsewhere seems a bit odd to me. Is it just more ergonomic than using subclassing?
There was a problem hiding this comment.
It's historic. I'm slowly getting rid of these.
I blame Copilot. :-) Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
I thought I got them all... Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
This simplifies a few lines in the code generator.
|
I'll just land once tests pass. The |
One deviation from the DSL spec:
macrocan build on bothinstandop.Also some other random improvements: